Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow smart contract verification #1800

Merged
merged 40 commits into from
Aug 4, 2020
Merged

Conversation

shargon
Copy link
Member

@shargon shargon commented Jul 28, 2020

Required for neo-project/neo-node#628 (comment)

  • Allow CheckWitness during Verification.
  • Allow SmartContract Verification.
  • Allow static varibles during Verification

@meevee98
Copy link
Contributor

Using the same test used in neo-project/neo-node#628 (comment):

I was able to sign and relay the transaction, but the transfer itself doesn't execute.
image

This only happened when transferring from smart contracts. I'm not sure what's causing the problem, but Transaction.Verify is returning Insufficient Funds even when the smart contract has the requested funds to transfer.
image

@shargon
Copy link
Member Author

shargon commented Jul 29, 2020

@meevee98 please take a look again.

I think that it's fixed

image

@ProDog
Copy link
Contributor

ProDog commented Jul 29, 2020

@shargon ,I have tested and it's error, NLtxB5cz8kC3ir1JD4nrY1bq7YawCr7Gba is contract address added by import watchonly, then I transfer GAS from this address.

image

image

@shargon
Copy link
Member Author

shargon commented Jul 29, 2020

Could you show me your script and manifest?

I tested it with this code, PUSH1 and verify in offset 0

[ConsoleCommand("deploy-test")]
        private void DeployTest()
        {
            var nef = new NefFile()
            {
                Compiler = "",
                Script = new byte[] { (byte)OpCode.PUSH1 },
                Version = new Version(3, 1, 1, 1),
            };
            nef.ScriptHash = nef.Script.ToScriptHash();
            nef.CheckSum = NefFile.ComputeChecksum(nef);

            ContractManifest cf = new ContractManifest()
            {
                Abi = new ContractAbi()
                {
                    Events = new ContractEventDescriptor[0],
                    Hash = nef.ScriptHash,
                    Methods = new ContractMethodDescriptor[]
                     {
                         new ContractMethodDescriptor()
                         {
                              Name="verify",
                               Offset=0,
                                Parameters=new ContractParameterDefinition[0],
                                 ReturnType= ContractParameterType.Boolean
                         }
                     }
                },
                Extra = null,
                Features = ContractFeatures.Payable,
                SafeMethods = WildcardContainer<string>.Create(),
                Groups = new ContractGroup[0],
                Permissions = new ContractPermission[0],
                SupportedStandards = new string[0],
                Trusts = WildcardContainer<UInt160>.Create()
            };

            File.WriteAllBytes("C:\\temporal\\verify.nef", nef.ToArray());
            File.WriteAllBytes("C:\\temporal\\verify.json", Encoding.UTF8.GetBytes(cf.ToJson().ToString()));

            OnDeployCommand("C:\\temporal\\verify.nef", "C:\\temporal\\verify.json");
        }

@ProDog
Copy link
Contributor

ProDog commented Jul 29, 2020

A simplified contract.

script(hex string):

0dce017b2267726f757073223a5b5d2c226665617475726573223a7b2273746f72616765223a747275652c2270617961626c65223a747275657d2c22737570706f727465647374616e6461726473223a5b5d2c22616269223a7b2268617368223a22307837316564626134656536653062306363613564346239626334333433623335613837353839666531222c226d6574686f6473223a5b7b226e616d65223a22766572696679222c22706172616d6574657273223a5b5d2c226f6666736574223a302c2272657475726e74797065223a22426f6f6c65616e227d2c7b226e616d65223a227465737431222c22706172616d6574657273223a5b5d2c226f6666736574223a31332c2272657475726e74797065223a22427974654172726179227d2c7b226e616d65223a225f696e697469616c697a65222c22706172616d6574657273223a5b5d2c226f6666736574223a32312c2272657475726e74797065223a22566f6964227d5d2c226576656e7473223a5b5d7d2c227065726d697373696f6e73223a5b7b22636f6e7472616374223a222a222c226d6574686f6473223a222a227d5d2c22747275737473223a5b5d2c22736166656d6574686f6473223a5b5d2c226578747261223a6e756c6c7d0c5d5701005f0041f827ec8c7068405701005f0170684056030c141c0357464b777ecf6b5f3ac3893ace1f8b1621f6600c14897720d8cd76f4f00abfa37c0edd889c208fde9b610c141c0357464b777ecf6b5f3ac3893ace1f8b1621f6624041ce352c85

manifest:

{"groups":[],"features":{"storage":true,"payable":true},"abi":{
    "hash":"0x71edba4ee6e0b0cca5d4b9bc4343b35a87589fe1",
    "methods":
    [
        {
            "name":"verify",
            "offset":"0",
            "parameters":
            [
            ],
            "returntype":"Boolean"
        },
        {
            "name":"test1",
            "offset":"13",
            "parameters":
            [
            ],
            "returntype":"ByteArray"
        },
        {
            "name":"_initialize",
            "offset":"21",
            "parameters":
            [
            ],
            "returntype":"Void"
        }
    ],
    "events":
    [
    ]
},"permissions":[{"contract":"*","methods":"*"}],"trusts":[],"safemethods":[],"supportedstandards":[],"extra":null}

@shargon
Copy link
Member Author

shargon commented Jul 29, 2020

I think that the problem could be that you use static variables and _initialize it's not called during verify. do you have the source?

@shargon
Copy link
Member Author

shargon commented Jul 29, 2020

Also _initialize offset points to STLOC5 instead of INITSLOT :S

@shargon
Copy link
Member Author

shargon commented Jul 29, 2020

@ProDog plase take a look again, now you should be able to use a static variable during verify, but check this error please #1800 (comment)

@ProDog
Copy link
Contributor

ProDog commented Jul 29, 2020

I think that the problem could be that you use static variables and _initialize it's not called during verify. do you have the source?

Yes, I will check it.

My contract:

 [Features(ContractFeatures.HasStorage | ContractFeatures.Payable)]
    public partial class APITest : SmartContract
    {        
        private static byte[] Owner = "NNU67Fvdy3LEQTM374EJ9iMbCRxVExgM8Y".ToScriptHash();
        public static bool Verify()
        {
            return Runtime.CheckWitness(Owner);
        }
     }

@ProDog
Copy link
Contributor

ProDog commented Jul 29, 2020

A new error:

System.InvalidOperationException: 'Cannot call this SYSCALL with the flag None.'

image

image

@shargon
Copy link
Member Author

shargon commented Jul 29, 2020

The problem was that verify can't call CheckWitness because require AllowStates, I changed to ReadOnly it should work, but @erikzhang we should reverify the transaction before persist? the states can be different.

@ProDog
Copy link
Contributor

ProDog commented Jul 30, 2020

New error:

image

@shargon
Copy link
Member Author

shargon commented Jul 30, 2020

@ProDog it was signed by NNU67Fvdy3LEQTM374EJ9iMbCRxVExgM8Y? we should be able to sign with multiple accounts, but we only have one from ... maybe we can change this to be an array?

@ProDog
Copy link
Contributor

ProDog commented Jul 30, 2020

@ProDog it was signed by NNU67Fvdy3LEQTM374EJ9iMbCRxVExgM8Y? we should be able to sign with multiple accounts, but we only have one from ... maybe we can change this to be an array?

Yes, so signers doesn't contains it.

@shargon
Copy link
Member Author

shargon commented Jul 30, 2020

Could you try with neo-project/neo-node@c186c36 or with invoke that allow multiple signers?

src/neo/Wallets/Wallet.cs Outdated Show resolved Hide resolved
erikzhang
erikzhang previously approved these changes Aug 3, 2020
@shargon
Copy link
Member Author

shargon commented Aug 3, 2020

@ProDog could you do the last test?

@shargon shargon requested a review from ProDog August 3, 2020 11:12
@@ -250,12 +251,12 @@ public Transaction MakeTransaction(TransferOutput[] outputs, UInt160 from = null
else
{
if (!Contains(from))
throw new ArgumentException($"The address {from.ToString()} was not found in the wallet");
throw new ArgumentException($"The address {from} was not found in the wallet");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So as neo-project/neo-node#628 (comment), we don't need this judgment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@ProDog
Copy link
Contributor

ProDog commented Aug 4, 2020

Another usage scenario, we use invoke to transfer asset from a verify contract, and contract is sender, like:

invoke nep5Hash transfer [{"type":"Hash160","value":contractHash},{"type":"Hash160","value":toAddress},{"type":"Integer","value":"10"}] contractHash verifyficationAccount

So it shouldn't be WitnessScope.FeeOnly:

Scopes = WitnessScope.FeeOnly

Now the CLI can't specified the sender to invoke command, I think we can add.

Copy link
Contributor

@ProDog ProDog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested OK.

@shargon shargon merged commit 64189a9 into neo-project:master Aug 4, 2020
@shargon shargon deleted the allow-sc-verify branch August 4, 2020 10:29
ixje added a commit to CityOfZion/neo-mamba that referenced this pull request Jan 15, 2021
cloud8little pushed a commit to cloud8little/neo that referenced this pull request Jan 24, 2021
* Allow sc verify

* Create DeployedContract

* Fix fee

* Fix

* Change to exceptions

* Allow static variables during verification

* Simplify DeployedContract

* Change verify call flags

* Change to ReadOnly

* Add error description

* Allow cosigners in makeTransaction

* Verify as APPCALL

* Fix fee

* Revert "Fix fee"

This reverts commit e8d59a9.

* Revert "Verify as APPCALL"

This reverts commit d8bf588.

* Auto stash before revert of "Allow cosigners in makeTransaction"

* None

* Add comment

* Fix CustomGroups

* Fix UT

* Update DeployedContract.cs

* Optimize MakeTransaction()

* Update Wallet.cs

* Fix

* Fix fee calculation

* Optimize CalculateNetworkFee()

* Try sc verification when the account it's null

* Update Wallet.cs

* Update Wallet.cs

* Add true verify test

* Update Wallet.cs

* Remove check account in wallet

* Fix UT

Co-authored-by: erikzhang <erik@neo.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants